Skip to content

DMX Output - Runtime Config & replace SparkFunDMX with esp_dmx#5287

Closed
netmindz wants to merge 184 commits intowled:mainfrom
netmindz:dmx-output-esp_dmx
Closed

DMX Output - Runtime Config & replace SparkFunDMX with esp_dmx#5287
netmindz wants to merge 184 commits intowled:mainfrom
netmindz:dmx-output-esp_dmx

Conversation

@netmindz
Copy link
Member

@netmindz netmindz commented Jan 10, 2026

Allow the DMX Output pin to be set at runtime, not need the user to hack around in the source code
Also swap the DMX Output to the same driver as DMX Input, leaving only 8266 to use ESPDMX

Summary by CodeRabbit

  • New Features

    • Optional DMX output: configure and persist a DMX output pin; realtime DMX controls added to settings.
    • WPA‑Enterprise WiFi fields supported in WiFi settings.
    • New “Pin Info” settings page showing GPIO usage/status.
    • New visual effects: PS Comet and several new user FX.
  • Refactor

    • DMX output stack redesigned to use pin-based initialization and a unified DMX backend.
  • Documentation

    • Updated contributor docs, issue template options, and new BLE/installation notes.

willmmiles and others added 12 commits November 27, 2025 20:31
Many of these functions are internal, and we can reduce RAM by caching
the binary value instead.
Co-authored-by: netmindz <442066+netmindz@users.noreply.github.com>
Co-authored-by: netmindz <442066+netmindz@users.noreply.github.com>
Add a field to the OTA metadata structure indicating the oldest base
version it's safe to install this update /from/.  This provides a clear
path forward in case there are incompatibilities, eg. some case
(bootloader compatibility) where 0.16.0 cannot be installed safely from
0.15.2, but a transitional 0.15.3 can arrange the groundwork.
If we cap the acceptable version, it becomes impossible to increase it:
older firmwares will reject it.  Instead we must guarantee
backwards compatibility so long as the magic number is the same.
If a breaking change is required in the future, a new magic number
should be used to identify it.
Report the older structure version until sufficient penetration
of the previous patch.
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 10, 2026

Walkthrough

Large refactor and feature bundle: adds DMX pin-configurable output and UI, introduces dynarray linker-based usermod registration, rewrites bus/PolyBus allocation and memory accounting, converts many FX/effect functions to void, tightens symbol linkage, replaces numeric JSON buffer lock IDs with named constants, updates many web UI pages and build scripts, and adjusts OTA/bootloader metadata and verification.

Changes

Cohort / File(s) Summary
Build & CI, tooling
platformio.ini, pio-scripts/dynarray.py, pio-scripts/load_usermods.py, pio-scripts/validate_modules.py, tools/dynarray_espressif32.ld, platformio_override.sample.ini, package.json, .github/workflows/*, .gitignore
Added global DMX build flag; new post-build dynarray script; improved usermod loading/validation; added linker fragment for dynarray; updated esp32 platform version and new env; bumped package version; adjusted CI changelog action; added ignore rules.
Dynarray / usermod registration
wled00/dynarray.h, um_manager.cpp, pio-scripts/dynarray.py, tools/dynarray_espressif32.ld
Introduce DECLARE_DYNARRAY/DYNARRAY_MEMBER macros, switch usermod registration to dynarray members, add linker script fragment and build script to inject linker flags.
DMX subsystem & drivers
wled00/dmx_output.h, wled00/dmx_output.cpp, wled00/src/dependencies/dmx/ESPDMX.h, .../ESPDMX.cpp, .../SparkFunDMX.*, wled00/wled.h, wled00/wled.cpp
Replace SparkFunDMX with DMXOutput (pin-parameterized API), change initDMXOutput to accept pin, refactor ESPDMX to init with sendPin, remove SparkFunDMX sources, expose dmxOutputPin, and update call sites and pin allocation.
Bus / PolyBus / BusManager / FX engine
wled00/bus_manager.cpp, wled00/bus_manager.h, wled00/bus_wrapper.h, wled00/FX.h, wled00/FX_fcn.cpp, wled00/bus_*...
Major API and behavior changes: driverType per-bus, dynamic channel allocation, placeholder buses, memUsage model changed, PolyBus channel-tracking added, effect function pointer type changed (uint16_t→void), finalizeInit/service logic updated, many constructors/signatures adjusted.
FX, particle systems, and many effect usermods
wled00/FXparticleSystem.*, usermods/user_fx/*, usermods/PS_Comet/*, usermods/TetrisAI_v2/*, usermods/*
Particle system API changes (updateFire), convert many mode_* functions to void, add new usermods (PS_Comet, user_fx effects), update TetrisAI and other usermods for new effect signatures and configuration persistence.
JSON locking, config, and DMX/Pin persistence
wled00/json.cpp, wled00/cfg.cpp, wled00/set.cpp, wled00/presets.cpp, wled00/mqtt.cpp, many wled00/* files
Replaced numeric JSON buffer lock IDs with named JSON_LOCK_* constants across many modules; added serializePins endpoint and persist dmxOutputPin; other config serialization updates (WPA enterprise fields, bus driver field).
Pin manager & Pin Info UI
wled00/pin_manager.h, wled00/pin_manager.cpp, wled00/data/settings_pininfo.htm, wled00/xml.cpp
Add DMX_OUTPUT owner, new pin capability flags, new PinManager APIs (getPinOwnerName, isAnalogPin, getButtonIndex), and new Pin Info web page plus XML/JS export wiring.
Web UI pages & assets
wled00/data/* (many files: settings_.htm, index., cpal.htm, pixelforge.htm, edit.htm, settings_sync.htm, settings_pininfo.htm, etc.), tools/cdata.js, wled00/js_*.h artifacts
Large UI rework: add/modify many settings pages and structure (.sec wrappers, new Pin Info, WPA enterprise UI, DMX output UI), dynamic script/resource loaders, add iro/omggif assets and generated headers, update CSS/icons and language attributes.
OTA / bootloader / metadata
wled00/ota_update.cpp, wled00/ota_update.h, wled00/wled_metadata.*
Rewrote bootloader SHA handling to stream-sized hashing, removed older bootloader API declarations, added safe_update_version to metadata, and tightened OTA compatibility checks.
Symbol visibility / static refactors
many wled00/*.cpp (ir.cpp, e131.cpp, improv.cpp, ntp.cpp, overlay.cpp, wled_serial.cpp, ws.cpp, remote.cpp, udp.cpp, etc.)
Converted numerous globals and helper functions to internal linkage (static), tightened inline/static declarations across many translation units to reduce exported symbols.
Miscellaneous
wled00/colors.cpp, wled00/const.h, wled00/file.cpp, wled00/util.cpp, usermods/*
Palette loading/gap logic, new constants (WLED_MAX_CUSTOM_PALETTE_GAP, relay delay, JSON_LOCK_* ids, platform channel limits), small bugfixes and behavior tweaks across utilities and usermods.

Estimated code review effort

🎯 5 (Critical) | ⏱️ ~120 minutes

Possibly related PRs

Suggested reviewers

  • softhack007
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

@netmindz netmindz changed the title Dmx output esp dmx DMX Outout - Runtime Config Jan 10, 2026
-D DECODE_LG=true
-DWLED_USE_MY_CONFIG
-D WLED_PS_DONT_REPLACE_FX ; PS replacement FX are purely a flash memory saving feature, do not replace classic FX until we run out of flash
-D WLED_ENABLE_DMX
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO: remove after testing, unless we can safely enable by default now it can be enabled at runtime?

@netmindz netmindz changed the title DMX Outout - Runtime Config DMX Output - Runtime Config Jan 10, 2026
@netmindz
Copy link
Member Author

I still need to test with actual hardware, but I think this should be pretty close to the required code

@netmindz netmindz changed the title DMX Output - Runtime Config DMX Output - Runtime Config & replace SparkFunDMX with esp_dmx Jan 10, 2026
softhack007 and others added 27 commits March 13, 2026 18:55
Clarify language requirement for code comments and documentation.
)

* add two new icons (folder, hammer), add text to tool buttons
- Remove unnecessary checks
- Remove unnecessary constructions
- Fix unsafe buffer-to-String
Allow OTA upgrade between ESP32_V4 and ESP32 release names
…utput-esp_dmx

# Conflicts:
#	wled00/data/settings_sync.htm
#	wled00/dmx_output.cpp
#	wled00/xml.cpp
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 18

Note

Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
wled00/json.cpp (1)

813-817: ⚠️ Potential issue | 🟠 Major

Keep info.arch stable; expose chip detail under a new key instead.

info.arch used to be a stable family identifier. Returning ESP.getChipModel() only on some ESP32 targets turns it into mixed-format values like "esp32" vs "ESP32-S3", which breaks exact-comparison clients. Preserve arch as the family value and publish the detailed chip model separately.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@wled00/json.cpp` around lines 813 - 817, Preserve info.arch as a stable
family identifier and add a separate key for the detailed chip model: always set
root[F("arch")] to the family string (e.g., "esp32" for ESP32 targets) instead
of conditionally using ESP.getChipModel(); then publish the full chip model
under a new key such as root[F("chip")] using ESP.getChipModel() (or equivalent)
so clients can still do exact comparisons on arch while accessing detailed model
via the new key.
♻️ Duplicate comments (3)
wled00/set.cpp (1)

476-478: ⚠️ Potential issue | 🟠 Major

Changing the DMX output pin still doesn't take effect at runtime.

This only updates dmxOutputPin; it doesn't release/reallocate the pin or reinitialize the DMX output driver, so the live session keeps using the old pin until reboot.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@wled00/set.cpp` around lines 476 - 478, The code only updates dmxOutputPin
but does not reinitialize the DMX driver, so change the handler that sets
dmxOutputPin (where request->arg(F("IDMO")).toInt() is used) to also reconfigure
the DMX output: stop/disable the current DMX instance, release the old pin, and
initialize/start the DMX driver on the new pin. Locate the DMX init/teardown
logic (e.g. functions used elsewhere like startDmxOutput()/stopDmxOutput(),
dmx.begin()/dmx.stop(), or the DMX driver init in setup) and invoke the teardown
then startup sequence after assigning dmxOutputPin; if such helper functions
don’t exist, add a small reinit routine that calls the driver’s stop and
begin/init with the new dmxOutputPin so the change takes effect immediately
without reboot.
wled00/dmx_output.cpp (2)

95-96: ⚠️ Potential issue | 🔴 Critical

uint8_t truncates half the DMX universe here.

handleDMXOutput() computes DMXAddr as an int, but write() narrows it to uint8_t before indexing dmxdata. Addresses above 255 will wrap to the wrong slot, and there is still no bounds guard on the array access.

🐛 Proposed fix
-void DMXOutput::write(uint8_t channel, uint8_t value) {
-  dmxdata[channel] = value;
+void DMXOutput::write(uint16_t channel, uint8_t value) {
+  if (channel == 0 || channel >= DMX_PACKET_SIZE) return;
+  dmxdata[channel] = value;
 }

Update the declaration in wled00/dmx_output.h to match.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@wled00/dmx_output.cpp` around lines 95 - 96, The write method currently
narrows the DMX address and can wrap addresses >255 and lacks bounds checking;
change DMXOutput::write signature to accept a wider integer type (e.g., int or
size_t) to match how DMXAddr is computed in handleDMXOutput(), update the
declaration in wled00/dmx_output.h accordingly, and inside DMXOutput::write
validate the incoming address against the dmxdata buffer size (use the buffer
length or a constant like DMX_UNIVERSE_SIZE) before indexing to avoid
out-of-bounds writes.

72-75: ⚠️ Potential issue | 🟠 Major

Pass a string to %s in the allocation failure log.

PinManager::getPinOwner() returns the enum, not a C string, so this varargs call is undefined behavior. This PR already adds PinManager::getPinOwnerName(), which fits the log directly.

🐛 Proposed fix
   if (!pinAllocated) {
     DEBUG_PRINTF("DMXOutput: Error: Failed to allocate pin for DMX_OUTPUT. Pin already in use:\n");
-    DEBUG_PRINTF("In use by: %s\n", PinManager::getPinOwner(outputPin));
+    DEBUG_PRINTF("In use by: %s\n", PinManager::getPinOwnerName(outputPin));
     return;
   }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@wled00/dmx_output.cpp` around lines 72 - 75, The allocation-failure log is
currently passing an enum to a "%s" format which is undefined behavior; update
the second DEBUG_PRINTF call to pass the string-returning helper instead by
replacing PinManager::getPinOwner(outputPin) with
PinManager::getPinOwnerName(outputPin) (keep the "%s" format), or alternatively
use the new getPinOwnerName() in any combined message so the log receives a
C-string; modify the DEBUG_PRINTF invocation in the DMXOutput allocation failure
branch accordingly.
🟡 Minor comments (7)
usermods/pixels_dice_tray/BLE_REQUIREMENT.md-17-20 (1)

17-20: ⚠️ Potential issue | 🟡 Minor

Fix the sample copy command path.

As written, this assumes the current directory already contains platformio_override.ini.sample, but then climbs three levels for the destination. From usermods/pixels_dice_tray/, repo root is only two levels up, so the command overshoots and copies to the wrong place.

🛠️ Suggested doc fix
-1. **Copy the sample configuration:**
+1. **From the repository root, copy the sample configuration:**
    ```bash
-   cp platformio_override.ini.sample ../../../platformio_override.ini
+   cp usermods/pixels_dice_tray/platformio_override.ini.sample platformio_override.ini
    ```
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@usermods/pixels_dice_tray/BLE_REQUIREMENT.md` around lines 17 - 20, The cp
command in BLE_REQUIREMENT.md incorrectly uses ../../../ which overshoots the
repo root; update the sample copy command so it references the actual sample
file path and copies it into the intended destination (replace the existing cp
line that mentions ../../../ with a command that reads the sample from
usermods/pixels_dice_tray/platformio_override.ini.sample and copies it to
platformio_override.ini) so readers running the example from the repo root or
from within usermods/pixels_dice_tray get the correct file.
wled00/data/index.css-3-3 (1)

3-3: ⚠️ Potential issue | 🟡 Minor

Fix CSS lint error on url() argument quoting.

Line 3 currently violates function-url-quotes (Stylelint). Please quote the url(...) argument.

Suggested lint-safe change
-  src: url(data:font/woff2;charset=utf-8;base64,...);
+  src: url("data:font/woff2;charset=utf-8;base64,...");
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@wled00/data/index.css` at line 3, The src declaration uses an unquoted data
URL in the url(...) call which triggers Stylelint's function-url-quotes rule;
fix it by updating the src: url(...) token (the data:font/woff2... entry) to use
quoted URL syntax (i.e., url("...")) so the entire data URL argument is wrapped
in quotes while leaving the rest of the `@font-face` declaration unchanged.
wled00/data/settings_sec.htm-69-69 (1)

69-69: ⚠️ Potential issue | 🟡 Minor

Potential orphaned </span> tag.

Line 69 has a closing </span> tag, but there's no corresponding opening <span> tag visible in the OTA section. This appears to be a remnant from the old markup structure that should be removed.

Proposed fix
 		<i class="warn">&#9888; If you are using multiple VLANs (i.e. IoT or guest network) either set PIN or disable this option.<br>
-			Disabling this option will make your device less secure.</i><br></span>
+			Disabling this option will make your device less secure.</i><br>
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@wled00/data/settings_sec.htm` at line 69, The closing </span> in the OTA
section is orphaned (see the HTML fragment "Disabling this option will make your
device less secure.</i><br></span>"); remove the stray </span> so the markup
properly closes only the <i> tag and the <br>, or if intended, add the
corresponding opening <span> with the needed attributes—prefer removing the
stray </span> to restore valid HTML in settings_sec.htm.
wled00/data/common.js-140-160 (1)

140-160: ⚠️ Potential issue | 🟡 Minor

Remove misleading async marker or implement proper promise completion.

The uploadFile() function is marked async but doesn't return a meaningful promise—it resolves immediately after req.send() rather than when the upload completes. While current callers rely on the callback pattern (which still works), this creates a confusing contract for future maintainers and could lead to bugs if someone tries to await the function. Either remove the async keyword to preserve the synchronous callback-only semantics, or implement a proper promise that resolves when the upload succeeds or fails.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@wled00/data/common.js` around lines 140 - 160, uploadFile is marked async but
currently returns immediately; convert it into a proper promise-returning async
function by returning a new Promise that resolves/rejects when the
XMLHttpRequest completes: create and return new Promise((resolve,reject)=>{ set
req.addEventListener('load', ...) to call resolve(this.status < 400) (and still
call callback if provided), set req.addEventListener('error', ...) to call
reject(false) or resolve(false) consistently (and call callback), and remove the
implicit immediate completion—ensure fileObj.value = '' runs after
resolution/rejection; keep existing await usage (e.g., await file.text()) so
uploadFile remains async but now correctly represents upload completion.
usermods/user_fx/README.md-269-273 (1)

269-273: ⚠️ Potential issue | 🟡 Minor

Use SEGENV.call in this example, not SEGMENT.call.

call lives on the segment environment, so the current wording points readers at a nonexistent API member.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@usermods/user_fx/README.md` around lines 269 - 273, The README example
incorrectly references SEGMENT.call; update the snippet and text to use
SEGENV.call (the call counter lives on the segment environment) so replace any
mention of SEGMENT.call with SEGENV.call and ensure the explanatory sentence
refers to SEGENV.call incrementing on each function call.
wled00/ota_update.cpp-403-442 (1)

403-442: ⚠️ Potential issue | 🟡 Minor

Keep the bootloader SHA cache invalid on read/parse failures.

calculateBootloaderSHA256() ignores failed esp_flash_read() calls and BootloaderImageSizer errors, but still sets bootloaderSHA256CacheValid = true. That turns a short/corrupt readback into the authoritative hash until reboot, which is misleading for the UI and any code consuming this value.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@wled00/ota_update.cpp` around lines 403 - 442, calculateBootloaderSHA256
currently ignores esp_flash_read failures and BootloaderImageSizer parse errors
and always sets bootloaderSHA256CacheValid = true; change it to track any
failure (e.g., set a bool readFailed if esp_flash_read(...) != ESP_OK, and a
bool parseFailed if the sizer detects a corrupt/impossible size such that
totalSize == 0 or totalSize > BOOTLOADER_SIZE or other sizer-specific error),
and if either flag is true or totalHashLen == 0 then do not mark the cache
valid: clear/zero bootloaderSHA256Cache, ensure mbedtls_sha256_free(&ctx) is
called, and leave bootloaderSHA256CacheValid = false (return early). Update
calculateBootloaderSHA256 to only set bootloaderSHA256CacheValid = true when no
read/parsing failures occurred and totalHashLen > 0.
wled00/bus_manager.cpp-229-242 (1)

229-242: ⚠️ Potential issue | 🟡 Minor

ABL reintroduces the CCT-from-RGB / accurate-white bug.

This loop reads back the already auto-white-processed pixel and then calls calculateCCT(c, ...). When _cct < 0 and RGBW_MODE_AUTO_ACCURATE is active, that RGB no longer matches the original color, so WW/CW shifts as soon as ABL repaints the bus. The normal setPixelColor() path fixed this by preserving the pre-auto-white color; this path needs the same source data or cached WW/CW values.

Based on learnings: When "Calculate CCT from RGB" mode (_cct < 0) is combined with "Accurate white" mode (RGBW_MODE_AUTO_ACCURATE), approximateKelvinFromRGB() receives RGB values that have already been modified by autoWhiteCalc(), so the original color must be preserved.

🧹 Nitpick comments (9)
CONTRIBUTING.md (2)

61-66: Fix blank line inside blockquote.

Line 66 creates a blank line inside the caution blockquote, which triggers markdownlint MD028. This can cause inconsistent rendering across markdown parsers.

📝 Proposed formatting fix
 > [!CAUTION] 
 > Do not use "force-push" while your PR is open!
 > It has many subtle and unexpected consequences on our GitHub repository.
 > For example, we regularly lose review comments when the PR author force-pushes code changes. Our review bot (coderabbit) may become unable to properly track changes, it gets confused or stops responding to questions.
 > So, pretty please, do not force-push.
-
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@CONTRIBUTING.md` around lines 61 - 66, The blockquote starting with
"[!CAUTION]" contains an extra blank line before "Do not use "force-push" while
your PR is open!" which triggers markdownlint MD028; remove the empty line so
the blockquote content is contiguous (i.e., ensure "[!CAUTION]" is immediately
followed by the warning text) to fix the formatting and prevent inconsistent
Markdown rendering.

17-21: Minor markdown formatting: Extra spaces in blockquote.

The blockquote has multiple spaces after the > symbol, which triggers markdownlint MD027 warnings. While this may be intentional for visual formatting in GitHub's renderer, standard markdown style uses a single space.

📝 Proposed formatting fix
 > [!TIP]
->   **The easiest way to start your first PR**
->   When viewing a file in `wled/WLED`, click on the "pen" icon and start making changes.
->   When you choose to 'Commit changes', GitHub will automatically create a PR from your fork.
->   
+> **The easiest way to start your first PR**
+> When viewing a file in `wled/WLED`, click on the "pen" icon and start making changes.
+> When you choose to 'Commit changes', GitHub will automatically create a PR from your fork.
+> 
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@CONTRIBUTING.md` around lines 17 - 21, The blockquote lines starting with "> 
" (the line containing "**The easiest way to start your first PR**" and the
following lines including the image tag) have extra spaces after the ">" which
triggers markdownlint MD027; update those blockquote lines to use a single space
after ">" (e.g., change ">   **The easiest..." to "> **The easiest..." and
similarly remove the extra spaces before the image tag and subsequent lines) so
the blockquote follows standard markdown spacing.
usermods/usermod_v2_auto_save/usermod_v2_auto_save.cpp (1)

132-132: Replace the hardcoded 240 throttle with a named constant.

This literal is meaningful behavior and should be self-describing.

♻️ Suggested cleanup
+static constexpr unsigned long AUTOSAVE_UPDATE_GUARD_MS = 240UL;
 ...
-  if (!autoSaveAfterSec || !enabled || (autoSaveIgnorePresets && currentPreset>0) || (strip.isUpdating() && now - lastRun < 240)) return;
+  if (!autoSaveAfterSec || !enabled || (autoSaveIgnorePresets && currentPreset>0) || (strip.isUpdating() && now - lastRun < AUTOSAVE_UPDATE_GUARD_MS)) return;

Based on learnings: In WLED reviews, meaningful magic numbers should be replaced with defined constants to improve maintainability and reduce inconsistencies.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@usermods/usermod_v2_auto_save/usermod_v2_auto_save.cpp` at line 132, Replace
the magic number 240 in the autosave throttle check with a named constant (e.g.,
AUTO_SAVE_UPDATE_THROTTLE_SEC) to make the meaning explicit; declare the
constant at the top of usermod_v2_auto_save.cpp (or alongside other constants)
and update the condition that uses strip.isUpdating() && now - lastRun < 240 to
use the new constant (refer to autoSaveAfterSec, enabled, autoSaveIgnorePresets,
currentPreset, strip.isUpdating(), lastRun, now) so the throttle interval is
self-documenting and easy to change.
wled00/data/icons-ui/style.css (1)

4-6: Verify woff2 format removal is intentional.

The woff2 font format was removed from the @font-face sources. While woff and ttf provide broad compatibility, woff2 offers better compression. If this was unintentional, consider re-adding it.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@wled00/data/icons-ui/style.css` around lines 4 - 6, The `@font-face` src list
in style.css (the lines listing url('fonts/wled122.ttf...'),
url('fonts/wled122.woff...'), url('fonts/wled122.svg...')) no longer includes a
woff2 entry; confirm this removal was intentional and, if not, restore the woff2
source (e.g., add a url('fonts/wled122.woff2?...') format('woff2') entry) within
the same `@font-face` src order and ensure the referenced woff2 file exists in the
fonts assets and any cache-busting query string matches your build artifacts.
usermods/pov_display/pov_display.cpp (1)

38-38: Nit: Redundant return statement.

The trailing return; at the end of a void function is unnecessary since the function will return implicitly.

Optional cleanup
   s_lastLoadAttemptMs = nowMs;
   s_pov.loadImage(segName);
-  return;
 }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@usermods/pov_display/pov_display.cpp` at line 38, Remove the redundant
trailing `return;` from the end of the void function in pov_display.cpp (the
final statement shown as `return;` in the diff); simply delete that line so the
function returns implicitly rather than explicitly.
wled00/button.cpp (1)

379-382: Avoid hard-wiring relay enable to beginStrip() ordering.

The relay-on path now works only because an earlier handleOnOff(true) already put the pin into output mode. Re-establishing the mode here keeps relay enable self-contained and avoids future breakage if the init order or pin ownership changes.

♻️ Suggested tweak
       if (rlyPin>=0) {
-        // note: pinMode is set in first call to handleOnOff(true) in beginStrip()
         digitalWrite(rlyPin, rlyMde); // set to on state
+        pinMode(rlyPin, rlyOpenDrain ? OUTPUT_OPEN_DRAIN : OUTPUT);
         delay(RELAY_DELAY); // let power stabilize before sending LED data (`#346` `#812` `#3581` `#3955`)
       }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@wled00/button.cpp` around lines 379 - 382, The relay enable assumes pinMode
was already set elsewhere; make the relay setup self-contained by calling
pinMode(rlyPin, OUTPUT) before digitalWrite(rlyPin, rlyMde) in the block that
checks rlyPin>=0 (so the relay is reliably driven even if handleOnOff(true) or
beginStrip() order changes), then keep the existing delay(RELAY_DELAY)
afterwards; reference rlyPin, rlyMde, RELAY_DELAY, digitalWrite(), and the
related beginStrip()/handleOnOff setup when updating the code.
wled00/data/settings_pininfo.htm (1)

16-30: Consider polling interval for resource-constrained devices.

The 250ms polling interval provides responsive updates but may be aggressive for ESP8266 or devices under load. Since pin state typically doesn't change rapidly, a longer interval (e.g., 500ms-1000ms) could reduce CPU/network overhead while still providing adequate responsiveness.

💡 Optional: Use a longer polling interval
-				pinsTimer = setInterval(loadPins, 250);
+				pinsTimer = setInterval(loadPins, 500);
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@wled00/data/settings_pininfo.htm` around lines 16 - 30, The 250ms polling in
S() (pinsTimer = setInterval(loadPins, 250)) is too aggressive for
resource-constrained devices; change the interval used by setInterval to a
longer value (e.g., 500–1000 ms) or make it configurable, and keep the call site
referencing pinsTimer and loadPins so the existing start/stop logic remains
intact.
usermods/TetrisAI_v2/TetrisAI_v2.cpp (1)

8-8: Make noFlashOnClear file-local.

It is only used inside usermods/TetrisAI_v2/TetrisAI_v2.cpp, so exporting it as a global symbol adds namespace pollution for no benefit. static (or a TetrisAIUsermod member) keeps the linkage tight. Based on learnings: when C-style functions or variables are added in WLED and are not declared in a global header, prefer making them static unless they are intentionally global.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@usermods/TetrisAI_v2/TetrisAI_v2.cpp` at line 8, The global variable
noFlashOnClear should be made file-local to avoid exporting an unnecessary
symbol: change its declaration in TetrisAI_v2.cpp to have internal linkage
(e.g., prepend static: "static bool noFlashOnClear = false;") or move it into
the TetrisAIUsermod class as a private member and update all uses accordingly;
update any references in the file to use the new member name if moved into the
class.
usermods/usermod_v2_animartrix/usermod_v2_animartrix.cpp (1)

88-296: These effect shims can be file-local.

Every mode_* wrapper in this block is only registered from this translation unit, so exporting all of them as global symbols unnecessarily widens the namespace. Mark them static or wrap the block in an anonymous namespace, and normalize the touched block to the repo's 2-space C++ style while you're here.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@usermods/usermod_v2_animartrix/usermod_v2_animartrix.cpp` around lines 88 -
296, This block of mode_* wrapper functions (e.g., mode_Module_Experiment10,
mode_Zoom, mode_RGB_Blobs, mode_Rotating_Blob) are only used in this translation
unit and should be file-local to avoid exporting globals; make them static (or
enclose the entire block in an anonymous namespace) and reformat the block to
the project's 2-space C++ style while preserving the existing calls like
anim.initEffect() and anim.<EffectName>().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@pio-scripts/load_usermods.py`:
- Around line 57-93: _predict_dep_name currently ignores symlink:// and file://
(and other non-hosted URL schemes) and thus returns None for those entries;
update it so that after the existing hosted-Git branch (the
_URL_SCHEME_RE.match(entry) check) you still parse any scheme with
urlparse(entry) and extract the final path component (Path(parsed.path).name),
stripping trailing '/' and a '.git' suffix and returning None if empty; retain
the special-case branch that handles github.com/gitlab.com/bitbucket.com by
using the second path segment when available, and ensure the SSH branch
(_SSH_URL_RE) and '@' / plain registry branches keep their current behavior
(still strip fragments, versions, and '.git' as currently done).

In `@pio-scripts/validate_modules.py`:
- Around line 42-50: The current subprocess.run call invoking nm (in the block
that assigns result = subprocess.run(...) and nm_output = result.stdout) only
handles exceptions, so if nm exits with a non-zero returncode we may parse empty
stdout and erroneously fail; update the logic to check result.returncode after
the subprocess.run call and treat non-zero as a tool failure: log a warning
including returncode and stderr, then return the conservative pass set (same as
in the exception branch) instead of proceeding to parse nm_output; ensure you
reference result.returncode and result.stderr when building the warning message
and preserve the existing conservative-return behavior used for
subprocess.TimeoutExpired/FileNotFoundError/OSError.

In `@usermods/PS_Comet/PS_Comet.cpp`:
- Line 4: The global nextCometCreationTime must be moved into segment-local
effect state so each segment running mode_pscomet() has its own throttle; stop
using the translation-unit global nextCometCreationTime and instead store the
deadline in SEGENV.step (or in the per-segment extra bytes after PSdataEnd) and
initialize it in the effect initialization path, then reference and update that
per-segment field inside mode_pscomet() wherever nextCometCreationTime is
currently read or written so reinitialization and separate segment instances do
not share the same timer.

In `@usermods/TetrisAI_v2/tetrisbag.h`:
- Around line 89-92: The loop in queuePiece() shifts elements then assigns to
piecesQueue[piecesQueue.size() - 1], which underflows when piecesQueue is empty;
add an early guard at the start of queuePiece() (e.g., if (piecesQueue.empty())
return;) or otherwise ensure queueLength > 0 before running the loop and the
final assignment so you never compute size()-1 on an empty vector; update the
function that contains the for-loop and the call to getNextPiece() to use this
guard.

In `@usermods/user_fx/user_fx.cpp`:
- Around line 1131-1147: The 4-bit hash in SEGENV.aux1 is too small and causes
frequent collisions; compute and keep a larger fingerprint (e.g., full 16-bit
textHash) and persist it in a wider field (use SEGENV.data or another available
SEGENV field) instead of truncating to 4 bits. Change the code that reads
lastHashBits from SEGENV.aux1 to read the full stored hash from SEGENV.data (or
the chosen field), compute a 16-bit textHash in the loop (keep textHash as
uint16_t), set currentHash = textHash (or a simple reduced-but-still-large slice
like top 12 bits), compare that against the stored value when computing
textChanged, and ensure when rebuilding you update the stored hash in
SEGENV.data (and stop packing the hash into SEGENV.aux1). Update references
around textHash, currentHashBits/lastHashBits, needsRebuild and where
SEGENV.aux1 was previously written so the new larger hash is saved and loaded
consistently (affects mode_morsecode / SEGMENT.name change detection).

In `@usermods/usermod_v2_auto_save/usermod_v2_auto_save.cpp`:
- Around line 132-133: When the early-return path triggered by
autoSaveIgnorePresets && currentPreset>0 runs, it must cancel any pending
autosave and resync the tracked state before returning; update the block around
the condition that checks autoSaveAfterSec, enabled, autoSaveIgnorePresets,
currentPreset, strip.isUpdating(), now and lastRun to clear the pending
autoSaveAfter timer/flag and refresh the known* tracking variables (e.g., known
color/state variables the autosaver uses) prior to returning so a
previously-armed autosave cannot later persist the preset state unexpectedly.

In `@wled00/bus_manager.cpp`:
- Around line 1190-1199: The current code inserts BusPlaceholder instances into
BusManager::busses which preserves the original bus type and causes unsafe
downcasts (e.g., static_cast<BusDigital*>(bus.get())->isI2S()) and premature
loop termination on checks like bus->isOk(); fix by either (A) never pushing
BusPlaceholder into BusManager::busses (create placeholders in a separate
container or only keep real Bus* in busses) or (B) add a reliable discriminator
on Bus (e.g., enum BusKind or bool isPlaceholder()) and update every downcast
and loop to check that discriminator before casting or treating the bus as real;
audit and guard all uses such as BusPlaceholder, BusManager::busses,
Bus::isVirtual, Bus::isDigital, the static_cast<BusDigital*> sites, the
virtual-cast to BusNetwork, and FX_fcn.cpp checks of bus->isOk() so placeholders
are skipped or handled safely.

In `@wled00/bus_wrapper.h`:
- Around line 1389-1398: The current logic in the I2S assignment block
(_i2sChannelsAssigned, offset) force-coerces later bus pixel type t to the first
lane's type via _parallelBusItype and flips _useParallelI2S, which silently
creates mismatched pixel-type buses; instead detect when a later request's t
differs from _parallelBusItype and reject/abort creation (or fall back to RMT)
rather than overwriting t: update the branch that handles offset==1 to compare t
vs _parallelBusItype and, on mismatch, return an error/false or switch to the
RMT path, keeping _useParallelI2S only for compatible types and preserving the
original t when compatible.

In `@wled00/cfg.cpp`:
- Around line 110-119: The current code always writes default/empty
WPA-Enterprise values into multiWiFi[n], causing silent downgrades when the JSON
omits those keys; change deserialize logic so encType, enterpriseAnonIdentity
and enterpriseIdentity are only overwritten when the corresponding JSON keys are
present/non-empty—use the existing wifi JSON checks (e.g., test wifi["e_ident"],
wifi["e_anon_ident"], and wifi[F("enc_type")] or CJSON result) and only call
multiWiFi[n].encryptionType = encType and strlcpy(...) when a value was actually
provided by getStringFromJson/CJSON, otherwise leave
multiWiFi[n].enterpriseAnonIdentity, multiWiFi[n].enterpriseIdentity and
multiWiFi[n].encryptionType unchanged.

In `@wled00/data/cpal/cpal.htm`:
- Around line 117-121: When toggling preview via the gId('chkPreview') change
handler, the current logic calls applyLED() or sends
requestJson({seg:{frz:false}}), which only unfreezes currently selected segments
and leaves the previously previewed main segment frozen; modify the handler and
related preview code to cache the previewed segment IDs (e.g., store in a
variable like previewedSegIds when applyLED() is used for previewing) and when
turning preview off (or on page leave) call requestJson for each cached id with
explicit objects like {seg:{id:<cachedId>, frz:false}} (or a single request with
an array of ids if the API supports it), then clear the cache; apply the same
change at the other occurrence noted (around lines 693–695) to ensure previewed
segments are explicitly unfrozen.

In `@wled00/data/edit.htm`:
- Around line 111-136: The timeout branch in loadFiles currently calls cb() as
if the script loaded, which can start dependent work prematurely; change the
timeout handler (tm) inside loadFiles so it invokes the script's error path
instead of cb() — e.g., call the same onerror logic (or invoke s.onerror()) so
the retry/failure logic runs (and eventually calls cb() only when appropriate).
Update references in loadFiles (tm, s.onload, s.onerror, cb) so the timeout
triggers the error/retry flow and thus prevents loadResources/S from being
called prematurely.

In `@wled00/data/index.js`:
- Around line 46-62: The current loadIro IIFE sets l.src = 'iro.js' before
onLoad populates location info, causing file: mode to try a local file and retry
indefinitely; change the flow so the page initializes first (wait for onLoad to
run or check d.readyState) and only then call a refactored loadIro that builds
l.src from the resolved location/IP (use the populated loc/locip or a canonical
absolute URL) and creates cpick; remove the tight infinite retry by adding a
graceful fallback (stop after a few retries or detect file: scheme and skip
loading the remote script), and ensure errors call a single failure handler
instead of continuously retrying so the IP prompt can appear when appropriate.

In `@wled00/data/settings_leds.htm`:
- Around line 147-150: The current save path contains a hard-stop when bquot >
100 (the block that alerts "Too many LEDs! Can't handle that!" and returns
false), which prevents saving oversized but recoverable configs; remove that
early return and instead only show warnings (preserve the existing alerts for
bquot > 80 and the additional messages referencing maxM) so the form can proceed
and allow finalizeInit() logic to trim bus lengths; ensure no other code paths
rely on that return to avoid proceeding with completely invalid data.

In `@wled00/dmx_input.cpp`:
- Around line 151-158: The early return on invalid inputPortNum leaks previously
allocated GPIO pins; either validate inputPortNum before any pin allocation
(move the inputPortNum check above the pin-allocation code) or, if you keep the
current order, free/deallocate the allocated pins before the early return (call
the existing pin cleanup routine or add a deallocatePins()/releasePins() call)
so that when the block around this->inputPortNum sets default and returns the
pins are not left reserved.

In `@wled00/dmx_output.cpp`:
- Around line 82-100: The DMX API calls use the v4.1.0 signatures: replace the
current three-arg dmx_driver_install call in DMXOutput::init with the four-arg
form expected by esp_dmx (pass dmxPort, config (not &config), a personalities
pointer (use nullptr or an empty array) and a personality count (0) to satisfy
the new signature), and change the dmx_send call in DMXOutput::update to the
single-argument form dmx_send(dmxPort); keep using dmx_write(dmxPort, dmxdata,
DMX_PACKET_SIZE) and dmx_set_pin(dmxPort, outputPin, -1, -1) as before but
ensure the types match the updated API.

In `@wled00/FX_fcn.cpp`:
- Around line 1165-1188: The code counts i2sBusCount from raw
BusConfig.driverType before fallbacks and then later double-counts bus memory
(memUsage + getBusSize), causing use_placeholder to trigger incorrectly; fix by
(1) moving the I2S detection to use the resolved runtime driver (call
BusManager::getI(...) or resolve the bus via BusManager before counting) and
derive useParallelI2S from that resolved driver instead of BusConfig.driverType,
and (2) remove the duplicate size addition (do not add getBusSize() on top of
BusConfig::memUsage() or adjust memUsage so it only counts each bus once) so
that mem and use_placeholder are computed from the same, non-duplicated values
(refer to busConfigs, i2sBusCount, Bus::isDigital, Bus::is2Pin, driverType,
BusManager::getI, useParallelI2S, BusConfig::memUsage, getBusSize,
use_placeholder).

In `@wled00/wled_metadata.cpp`:
- Around line 131-137: The bufToString template leaves sbuf uninitialized past
real_len causing nondeterministic reads; in the bufToString(const char
(&buf)[len]) function, after memcpy(..., real_len) set sbuf[real_len] = '\0'
(instead of sbuf[len]) so the copied string is properly null-terminated and
String(sbuf) only reads the intended bytes.
- Around line 56-66: The metadata struct WLED_BUILD_DESCRIPTION is emitting a
hard-coded version of 1 which prevents shouldAllowOTA() from checking
safe_update_version; change the second field (currently
"/*WLED_CUSTOM_DESC_VERSION*/ 1") to the actual v2 metadata version (e.g.,
replace with WLED_CUSTOM_DESC_VERSION or the literal 2) so binaries produced
include desc_version > 1 and trigger the safe-update gate (update the
initializer in WLED_BUILD_DESCRIPTION).

---

Outside diff comments:
In `@wled00/json.cpp`:
- Around line 813-817: Preserve info.arch as a stable family identifier and add
a separate key for the detailed chip model: always set root[F("arch")] to the
family string (e.g., "esp32" for ESP32 targets) instead of conditionally using
ESP.getChipModel(); then publish the full chip model under a new key such as
root[F("chip")] using ESP.getChipModel() (or equivalent) so clients can still do
exact comparisons on arch while accessing detailed model via the new key.

---

Minor comments:
In `@usermods/pixels_dice_tray/BLE_REQUIREMENT.md`:
- Around line 17-20: The cp command in BLE_REQUIREMENT.md incorrectly uses
../../../ which overshoots the repo root; update the sample copy command so it
references the actual sample file path and copies it into the intended
destination (replace the existing cp line that mentions ../../../ with a command
that reads the sample from
usermods/pixels_dice_tray/platformio_override.ini.sample and copies it to
platformio_override.ini) so readers running the example from the repo root or
from within usermods/pixels_dice_tray get the correct file.

In `@usermods/user_fx/README.md`:
- Around line 269-273: The README example incorrectly references SEGMENT.call;
update the snippet and text to use SEGENV.call (the call counter lives on the
segment environment) so replace any mention of SEGMENT.call with SEGENV.call and
ensure the explanatory sentence refers to SEGENV.call incrementing on each
function call.

In `@wled00/data/common.js`:
- Around line 140-160: uploadFile is marked async but currently returns
immediately; convert it into a proper promise-returning async function by
returning a new Promise that resolves/rejects when the XMLHttpRequest completes:
create and return new Promise((resolve,reject)=>{ set
req.addEventListener('load', ...) to call resolve(this.status < 400) (and still
call callback if provided), set req.addEventListener('error', ...) to call
reject(false) or resolve(false) consistently (and call callback), and remove the
implicit immediate completion—ensure fileObj.value = '' runs after
resolution/rejection; keep existing await usage (e.g., await file.text()) so
uploadFile remains async but now correctly represents upload completion.

In `@wled00/data/index.css`:
- Line 3: The src declaration uses an unquoted data URL in the url(...) call
which triggers Stylelint's function-url-quotes rule; fix it by updating the src:
url(...) token (the data:font/woff2... entry) to use quoted URL syntax (i.e.,
url("...")) so the entire data URL argument is wrapped in quotes while leaving
the rest of the `@font-face` declaration unchanged.

In `@wled00/data/settings_sec.htm`:
- Line 69: The closing </span> in the OTA section is orphaned (see the HTML
fragment "Disabling this option will make your device less
secure.</i><br></span>"); remove the stray </span> so the markup properly closes
only the <i> tag and the <br>, or if intended, add the corresponding opening
<span> with the needed attributes—prefer removing the stray </span> to restore
valid HTML in settings_sec.htm.

In `@wled00/ota_update.cpp`:
- Around line 403-442: calculateBootloaderSHA256 currently ignores
esp_flash_read failures and BootloaderImageSizer parse errors and always sets
bootloaderSHA256CacheValid = true; change it to track any failure (e.g., set a
bool readFailed if esp_flash_read(...) != ESP_OK, and a bool parseFailed if the
sizer detects a corrupt/impossible size such that totalSize == 0 or totalSize >
BOOTLOADER_SIZE or other sizer-specific error), and if either flag is true or
totalHashLen == 0 then do not mark the cache valid: clear/zero
bootloaderSHA256Cache, ensure mbedtls_sha256_free(&ctx) is called, and leave
bootloaderSHA256CacheValid = false (return early). Update
calculateBootloaderSHA256 to only set bootloaderSHA256CacheValid = true when no
read/parsing failures occurred and totalHashLen > 0.

---

Duplicate comments:
In `@wled00/dmx_output.cpp`:
- Around line 95-96: The write method currently narrows the DMX address and can
wrap addresses >255 and lacks bounds checking; change DMXOutput::write signature
to accept a wider integer type (e.g., int or size_t) to match how DMXAddr is
computed in handleDMXOutput(), update the declaration in wled00/dmx_output.h
accordingly, and inside DMXOutput::write validate the incoming address against
the dmxdata buffer size (use the buffer length or a constant like
DMX_UNIVERSE_SIZE) before indexing to avoid out-of-bounds writes.
- Around line 72-75: The allocation-failure log is currently passing an enum to
a "%s" format which is undefined behavior; update the second DEBUG_PRINTF call
to pass the string-returning helper instead by replacing
PinManager::getPinOwner(outputPin) with PinManager::getPinOwnerName(outputPin)
(keep the "%s" format), or alternatively use the new getPinOwnerName() in any
combined message so the log receives a C-string; modify the DEBUG_PRINTF
invocation in the DMXOutput allocation failure branch accordingly.

In `@wled00/set.cpp`:
- Around line 476-478: The code only updates dmxOutputPin but does not
reinitialize the DMX driver, so change the handler that sets dmxOutputPin (where
request->arg(F("IDMO")).toInt() is used) to also reconfigure the DMX output:
stop/disable the current DMX instance, release the old pin, and initialize/start
the DMX driver on the new pin. Locate the DMX init/teardown logic (e.g.
functions used elsewhere like startDmxOutput()/stopDmxOutput(),
dmx.begin()/dmx.stop(), or the DMX driver init in setup) and invoke the teardown
then startup sequence after assigning dmxOutputPin; if such helper functions
don’t exist, add a small reinit routine that calls the driver’s stop and
begin/init with the new dmxOutputPin so the change takes effect immediately
without reboot.

---

Nitpick comments:
In `@CONTRIBUTING.md`:
- Around line 61-66: The blockquote starting with "[!CAUTION]" contains an extra
blank line before "Do not use "force-push" while your PR is open!" which
triggers markdownlint MD028; remove the empty line so the blockquote content is
contiguous (i.e., ensure "[!CAUTION]" is immediately followed by the warning
text) to fix the formatting and prevent inconsistent Markdown rendering.
- Around line 17-21: The blockquote lines starting with ">   " (the line
containing "**The easiest way to start your first PR**" and the following lines
including the image tag) have extra spaces after the ">" which triggers
markdownlint MD027; update those blockquote lines to use a single space after
">" (e.g., change ">   **The easiest..." to "> **The easiest..." and similarly
remove the extra spaces before the image tag and subsequent lines) so the
blockquote follows standard markdown spacing.

In `@usermods/pov_display/pov_display.cpp`:
- Line 38: Remove the redundant trailing `return;` from the end of the void
function in pov_display.cpp (the final statement shown as `return;` in the
diff); simply delete that line so the function returns implicitly rather than
explicitly.

In `@usermods/TetrisAI_v2/TetrisAI_v2.cpp`:
- Line 8: The global variable noFlashOnClear should be made file-local to avoid
exporting an unnecessary symbol: change its declaration in TetrisAI_v2.cpp to
have internal linkage (e.g., prepend static: "static bool noFlashOnClear =
false;") or move it into the TetrisAIUsermod class as a private member and
update all uses accordingly; update any references in the file to use the new
member name if moved into the class.

In `@usermods/usermod_v2_animartrix/usermod_v2_animartrix.cpp`:
- Around line 88-296: This block of mode_* wrapper functions (e.g.,
mode_Module_Experiment10, mode_Zoom, mode_RGB_Blobs, mode_Rotating_Blob) are
only used in this translation unit and should be file-local to avoid exporting
globals; make them static (or enclose the entire block in an anonymous
namespace) and reformat the block to the project's 2-space C++ style while
preserving the existing calls like anim.initEffect() and anim.<EffectName>().

In `@usermods/usermod_v2_auto_save/usermod_v2_auto_save.cpp`:
- Line 132: Replace the magic number 240 in the autosave throttle check with a
named constant (e.g., AUTO_SAVE_UPDATE_THROTTLE_SEC) to make the meaning
explicit; declare the constant at the top of usermod_v2_auto_save.cpp (or
alongside other constants) and update the condition that uses strip.isUpdating()
&& now - lastRun < 240 to use the new constant (refer to autoSaveAfterSec,
enabled, autoSaveIgnorePresets, currentPreset, strip.isUpdating(), lastRun, now)
so the throttle interval is self-documenting and easy to change.

In `@wled00/button.cpp`:
- Around line 379-382: The relay enable assumes pinMode was already set
elsewhere; make the relay setup self-contained by calling pinMode(rlyPin,
OUTPUT) before digitalWrite(rlyPin, rlyMde) in the block that checks rlyPin>=0
(so the relay is reliably driven even if handleOnOff(true) or beginStrip() order
changes), then keep the existing delay(RELAY_DELAY) afterwards; reference
rlyPin, rlyMde, RELAY_DELAY, digitalWrite(), and the related
beginStrip()/handleOnOff setup when updating the code.

In `@wled00/data/icons-ui/style.css`:
- Around line 4-6: The `@font-face` src list in style.css (the lines listing
url('fonts/wled122.ttf...'), url('fonts/wled122.woff...'),
url('fonts/wled122.svg...')) no longer includes a woff2 entry; confirm this
removal was intentional and, if not, restore the woff2 source (e.g., add a
url('fonts/wled122.woff2?...') format('woff2') entry) within the same `@font-face`
src order and ensure the referenced woff2 file exists in the fonts assets and
any cache-busting query string matches your build artifacts.

In `@wled00/data/settings_pininfo.htm`:
- Around line 16-30: The 250ms polling in S() (pinsTimer = setInterval(loadPins,
250)) is too aggressive for resource-constrained devices; change the interval
used by setInterval to a longer value (e.g., 500–1000 ms) or make it
configurable, and keep the call site referencing pinsTimer and loadPins so the
existing start/stop logic remains intact.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: d1610ede-c6d8-4616-be92-169479d0f724

📥 Commits

Reviewing files that changed from the base of the PR and between 48168ed and cf1f236.

⛔ Files ignored due to path filters (5)
  • package-lock.json is excluded by !**/package-lock.json
  • wled00/data/icons-ui/fonts/wled122.svg is excluded by !**/*.svg
  • wled00/data/icons-ui/fonts/wled122.ttf is excluded by !**/*.ttf
  • wled00/data/icons-ui/fonts/wled122.woff is excluded by !**/*.woff
  • wled00/data/icons-ui/fonts/wled122.woff2 is excluded by !**/*.woff2
📒 Files selected for processing (118)
  • .github/ISSUE_TEMPLATE/bug.yml
  • .github/copilot-instructions.md
  • .github/workflows/nightly.yml
  • .github/workflows/release.yml
  • .gitignore
  • CONTRIBUTING.md
  • package.json
  • pio-scripts/dynarray.py
  • pio-scripts/load_usermods.py
  • pio-scripts/validate_modules.py
  • platformio.ini
  • platformio_override.sample.ini
  • tools/cdata.js
  • tools/dynarray_espressif32.ld
  • usermods/Analog_Clock/Analog_Clock.cpp
  • usermods/PS_Comet/PS_Comet.cpp
  • usermods/PS_Comet/README.md
  • usermods/PS_Comet/library.json
  • usermods/Temperature/Temperature.cpp
  • usermods/TetrisAI_v2/TetrisAI_v2.cpp
  • usermods/TetrisAI_v2/gridbw.h
  • usermods/TetrisAI_v2/gridcolor.h
  • usermods/TetrisAI_v2/pieces.h
  • usermods/TetrisAI_v2/readme.md
  • usermods/TetrisAI_v2/tetrisai.h
  • usermods/TetrisAI_v2/tetrisbag.h
  • usermods/audioreactive/audio_reactive.cpp
  • usermods/audioreactive/audio_source.h
  • usermods/deep_sleep/deep_sleep.cpp
  • usermods/pixels_dice_tray/BLE_REQUIREMENT.md
  • usermods/pixels_dice_tray/led_effects.h
  • usermods/pixels_dice_tray/library.json
  • usermods/pov_display/pov_display.cpp
  • usermods/rgb-rotary-encoder/rgb-rotary-encoder.cpp
  • usermods/user_fx/README.md
  • usermods/user_fx/user_fx.cpp
  • usermods/usermod_v2_RF433/usermod_v2_RF433.cpp
  • usermods/usermod_v2_animartrix/usermod_v2_animartrix.cpp
  • usermods/usermod_v2_auto_save/usermod_v2_auto_save.cpp
  • wled00/FX.cpp
  • wled00/FX.h
  • wled00/FX_2Dfcn.cpp
  • wled00/FX_fcn.cpp
  • wled00/FXparticleSystem.cpp
  • wled00/FXparticleSystem.h
  • wled00/NodeStruct.h
  • wled00/bus_manager.cpp
  • wled00/bus_manager.h
  • wled00/bus_wrapper.h
  • wled00/button.cpp
  • wled00/cfg.cpp
  • wled00/colors.cpp
  • wled00/const.h
  • wled00/data/404.htm
  • wled00/data/common.js
  • wled00/data/cpal/cpal.htm
  • wled00/data/dmxmap.htm
  • wled00/data/edit.htm
  • wled00/data/icons-ui/HowTo_AddNewIcons.txt
  • wled00/data/icons-ui/Read Me.txt
  • wled00/data/icons-ui/demo-files/demo.css
  • wled00/data/icons-ui/demo.html
  • wled00/data/icons-ui/selection.json
  • wled00/data/icons-ui/style.css
  • wled00/data/index.css
  • wled00/data/index.htm
  • wled00/data/index.js
  • wled00/data/liveview.htm
  • wled00/data/liveviewws2D.htm
  • wled00/data/msg.htm
  • wled00/data/pixart/pixart.htm
  • wled00/data/pixelforge/pixelforge.htm
  • wled00/data/settings.htm
  • wled00/data/settings_2D.htm
  • wled00/data/settings_leds.htm
  • wled00/data/settings_pin.htm
  • wled00/data/settings_pininfo.htm
  • wled00/data/settings_sec.htm
  • wled00/data/settings_sync.htm
  • wled00/data/settings_time.htm
  • wled00/data/settings_ui.htm
  • wled00/data/settings_um.htm
  • wled00/data/settings_wifi.htm
  • wled00/data/style.css
  • wled00/data/update.htm
  • wled00/data/usermod.htm
  • wled00/data/welcome.htm
  • wled00/dmx_input.cpp
  • wled00/dmx_output.cpp
  • wled00/dynarray.h
  • wled00/e131.cpp
  • wled00/fcn_declare.h
  • wled00/file.cpp
  • wled00/improv.cpp
  • wled00/ir.cpp
  • wled00/json.cpp
  • wled00/mqtt.cpp
  • wled00/network.cpp
  • wled00/ntp.cpp
  • wled00/ota_update.cpp
  • wled00/ota_update.h
  • wled00/overlay.cpp
  • wled00/pin_manager.cpp
  • wled00/pin_manager.h
  • wled00/presets.cpp
  • wled00/remote.cpp
  • wled00/set.cpp
  • wled00/udp.cpp
  • wled00/um_manager.cpp
  • wled00/util.cpp
  • wled00/wled.cpp
  • wled00/wled.h
  • wled00/wled_metadata.cpp
  • wled00/wled_metadata.h
  • wled00/wled_serial.cpp
  • wled00/wled_server.cpp
  • wled00/ws.cpp
  • wled00/xml.cpp
💤 Files with no reviewable changes (4)
  • usermods/TetrisAI_v2/pieces.h
  • usermods/Analog_Clock/Analog_Clock.cpp
  • usermods/pixels_dice_tray/library.json
  • wled00/ota_update.h
✅ Files skipped from review due to trivial changes (24)
  • .gitignore
  • wled00/data/msg.htm
  • wled00/data/liveview.htm
  • wled00/data/dmxmap.htm
  • wled00/data/usermod.htm
  • wled00/data/settings_pin.htm
  • wled00/data/404.htm
  • wled00/data/icons-ui/HowTo_AddNewIcons.txt
  • wled00/data/settings.htm
  • wled00/data/liveviewws2D.htm
  • usermods/PS_Comet/library.json
  • wled00/data/welcome.htm
  • wled00/network.cpp
  • wled00/data/style.css
  • wled00/data/pixart/pixart.htm
  • usermods/deep_sleep/deep_sleep.cpp
  • .github/ISSUE_TEMPLATE/bug.yml
  • wled00/data/icons-ui/demo-files/demo.css
  • wled00/data/settings_time.htm
  • package.json
  • wled00/data/settings_um.htm
  • wled00/NodeStruct.h
  • wled00/e131.cpp
  • usermods/TetrisAI_v2/readme.md
🚧 Files skipped from review as they are similar to previous changes (5)
  • wled00/pin_manager.h
  • wled00/data/settings_sync.htm
  • wled00/xml.cpp
  • platformio.ini
  • wled00/wled.cpp

Comment on lines +57 to +93
def _predict_dep_name(entry: str) -> str | None:
"""Predict the library name PlatformIO will assign to this dep (best-effort).

Accuracy relies on the library's manifest "name" matching the repo/package
name in the spec. This holds for well-authored libraries; the libArchive
check (which requires library.json) provides an early-failure safety net.
"""
return usermod_dir in Path(dep.src_dir).parents or str(dep.name).startswith("wled-")

## Script starts here
# Process usermod option
usermods = env.GetProjectOption("custom_usermods","")
entry = entry.strip()
# "LibName = <spec>" — name is given explicitly; always use it
m = _NAME_EQ_RE.match(entry)
if m:
return m.group(1).strip()
# URL scheme: extract name from path
if _URL_SCHEME_RE.match(entry):
parsed = urlparse(entry)
if parsed.netloc in ('github.com', 'gitlab.com', 'bitbucket.com'):
parts = [p for p in parsed.path.split('/') if p]
if len(parts) >= 2:
name = parts[1]
else:
name = Path(parsed.path.rstrip('/')).name.strip()
if name.endswith('.git'):
name = name[:-4]
return name or None
# SSH git URL: git@github.com:user/repo.git#tag → repo
if _SSH_URL_RE.match(entry):
path_part = entry.split(':', 1)[1].split('#')[0].rstrip('/')
name = Path(path_part).name
return (name[:-4] if name.endswith('.git') else name) or None
# Versioned registry: "owner/Name @ version" → Name
if '@' in entry:
name_part = entry.split('@')[0].strip()
return name_part.split('/')[-1].strip() if '/' in name_part else name_part
# Plain registry: "owner/Name" → Name
if re.match(r'^[^/\s]+/[^/\s]+$', entry):
return entry.split('/')[-1].strip()
return None
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
python - <<'PY'
from urllib.parse import urlparse
from pathlib import Path

samples = [
    "symlink:///tmp/example_usermod",
    "file:///tmp/example_usermod",
    "https://github.com/org/example_usermod.git",
]

for entry in samples:
    parsed = urlparse(entry)
    name = None
    if parsed.netloc in ("github.com", "gitlab.com", "bitbucket.com"):
        parts = [p for p in parsed.path.split("/") if p]
        if len(parts) >= 2:
            name = parts[1]
        else:
            name = Path(parsed.path.rstrip("/")).name.strip()
        if name.endswith(".git"):
            name = name[:-4]
    print(f"{entry} -> {name}")
PY

Repository: wled/WLED

Length of output: 189


🏁 Script executed:

# Search for symlink:// and file:// references in the codebase
rg "symlink://" --type py
rg "file://" --type py

Repository: wled/WLED

Length of output: 357


🏁 Script executed:

# Check how _custom_usermod_names is used and populated
rg "_custom_usermod_names" --type py -A 3 -B 3

Repository: wled/WLED

Length of output: 2207


🏁 Script executed:

# Examine the full context around _predict_dep_name usage (lines around 57-93, 96-102, 114-125)
cat -n pio-scripts/load_usermods.py | sed -n '50,130p'

Repository: wled/WLED

Length of output: 3569


🏁 Script executed:

# Check if there's documentation about custom_usermods format
rg -i "custom_usermod|symlink|file://" --type md

Repository: wled/WLED

Length of output: 4918


Fix _predict_dep_name() to handle symlink:// and file:// paths.

The code generates and uses symlink:// URLs internally (lines 131, 136), yet _predict_dep_name() fails to extract names from them, returning None instead. This prevents those modules from being registered in _custom_usermod_names and breaks the is_wled_module() check. The function should extract the directory name from all URL schemes, not just hosted Git providers.

The suggested fix extracts the final path component for symlink://, file://, and other schemes while preserving special handling for hosted Git repos:

Suggested fix
   if _URL_SCHEME_RE.match(entry):
     parsed = urlparse(entry)
-    if parsed.netloc in ('github.com', 'gitlab.com', 'bitbucket.com'):
-      parts = [p for p in parsed.path.split('/') if p]
-      if len(parts) >= 2:
-        name = parts[1]
-      else:
-        name = Path(parsed.path.rstrip('/')).name.strip()
-      if name.endswith('.git'):
-        name = name[:-4]
-      return name or None
+    path = parsed.path.split('#', 1)[0].rstrip('/')
+    parts = [p for p in path.split('/') if p]
+    if parsed.netloc in ('github.com', 'gitlab.com', 'bitbucket.com') and len(parts) >= 2:
+      name = parts[1]
+    else:
+      name = Path(path).name.strip()
+    if name.endswith('.git'):
+      name = name[:-4]
+    return name or None

Affects lines 96–102 and 114–125 as well.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@pio-scripts/load_usermods.py` around lines 57 - 93, _predict_dep_name
currently ignores symlink:// and file:// (and other non-hosted URL schemes) and
thus returns None for those entries; update it so that after the existing
hosted-Git branch (the _URL_SCHEME_RE.match(entry) check) you still parse any
scheme with urlparse(entry) and extract the final path component
(Path(parsed.path).name), stripping trailing '/' and a '.git' suffix and
returning None if empty; retain the special-case branch that handles
github.com/gitlab.com/bitbucket.com by using the second path segment when
available, and ensure the SSH branch (_SSH_URL_RE) and '@' / plain registry
branches keep their current behavior (still strip fragments, versions, and
'.git' as currently done).

Comment on lines +42 to +50
try:
result = subprocess.run(
[nm_path, "--defined-only", "-l", str(elf_path)],
capture_output=True, text=True, errors="ignore", timeout=120,
)
nm_output = result.stdout
except (subprocess.TimeoutExpired, FileNotFoundError, OSError) as e:
secho(f"WARNING: nm failed ({e}); skipping per-module validation", fg="yellow", err=True)
return {Path(b.build_dir).name for b in module_lib_builders} # conservative pass
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Handle non-zero nm exits before parsing stdout.

Only exceptions take the conservative-pass path today. If nm exists but exits non-zero, stdout can be empty and this turns into a false "No symbols..." build failure instead of being treated as a tool failure.

🐛 Proposed fix
     try:
         result = subprocess.run(
             [nm_path, "--defined-only", "-l", str(elf_path)],
             capture_output=True, text=True, errors="ignore", timeout=120,
         )
+        if result.returncode != 0:
+            secho(
+                f"WARNING: nm failed with exit code {result.returncode}; skipping per-module validation",
+                fg="yellow",
+                err=True,
+            )
+            return {Path(b.build_dir).name for b in module_lib_builders}
         nm_output = result.stdout
     except (subprocess.TimeoutExpired, FileNotFoundError, OSError) as e:
         secho(f"WARNING: nm failed ({e}); skipping per-module validation", fg="yellow", err=True)
         return {Path(b.build_dir).name for b in module_lib_builders}  # conservative pass
🧰 Tools
🪛 Ruff (0.15.6)

[error] 43-43: subprocess call: check for execution of untrusted input

(S603)

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@pio-scripts/validate_modules.py` around lines 42 - 50, The current
subprocess.run call invoking nm (in the block that assigns result =
subprocess.run(...) and nm_output = result.stdout) only handles exceptions, so
if nm exits with a non-zero returncode we may parse empty stdout and erroneously
fail; update the logic to check result.returncode after the subprocess.run call
and treat non-zero as a tool failure: log a warning including returncode and
stderr, then return the conservative pass set (same as in the exception branch)
instead of proceeding to parse nm_output; ensure you reference result.returncode
and result.stderr when building the warning message and preserve the existing
conservative-return behavior used for
subprocess.TimeoutExpired/FileNotFoundError/OSError.

#include "wled.h"
#include "FXparticleSystem.h"

unsigned long nextCometCreationTime = 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Keep comet spawn timing in segment-local state.

nextCometCreationTime is a translation-unit global, so every segment running mode_pscomet() shares the same spawn throttle and the deadline survives effect reinitialization. This needs to live in segment-local effect state (SEGENV.step or extra bytes after PSdataEnd), not in a global.

Also applies to: 47-55

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@usermods/PS_Comet/PS_Comet.cpp` at line 4, The global nextCometCreationTime
must be moved into segment-local effect state so each segment running
mode_pscomet() has its own throttle; stop using the translation-unit global
nextCometCreationTime and instead store the deadline in SEGENV.step (or in the
per-segment extra bytes after PSdataEnd) and initialize it in the effect
initialization path, then reference and update that per-segment field inside
mode_pscomet() wherever nextCometCreationTime is currently read or written so
reinitialization and separate segment instances do not share the same timer.

Comment on lines +89 to 92
for (uint8_t i = 1; i < piecesQueue.size(); i++) {
piecesQueue[i - 1] = piecesQueue[i];
}
piecesQueue[piecesQueue.size() - 1] = getNextPiece();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Guard queuePiece() against empty queues before indexing.

If piecesQueue is empty, Line 92 computes piecesQueue.size() - 1, underflows, and performs out-of-bounds access. Add an early return (or enforce queueLength > 0 before use).

Suggested fix
 void queuePiece()
 {
+    if (piecesQueue.empty()) return;
     //move vector to left
-    for (uint8_t i = 1; i < piecesQueue.size(); i++) {
+    for (uint8_t i = 1; i < piecesQueue.size(); i++) {
         piecesQueue[i - 1] = piecesQueue[i];
     }
     piecesQueue[piecesQueue.size() - 1] = getNextPiece();
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
for (uint8_t i = 1; i < piecesQueue.size(); i++) {
piecesQueue[i - 1] = piecesQueue[i];
}
piecesQueue[piecesQueue.size() - 1] = getNextPiece();
if (piecesQueue.empty()) return;
for (uint8_t i = 1; i < piecesQueue.size(); i++) {
piecesQueue[i - 1] = piecesQueue[i];
}
piecesQueue[piecesQueue.size() - 1] = getNextPiece();
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@usermods/TetrisAI_v2/tetrisbag.h` around lines 89 - 92, The loop in
queuePiece() shifts elements then assigns to piecesQueue[piecesQueue.size() -
1], which underflows when piecesQueue is empty; add an early guard at the start
of queuePiece() (e.g., if (piecesQueue.empty()) return;) or otherwise ensure
queueLength > 0 before running the loop and the final assignment so you never
compute size()-1 on an empty vector; update the function that contains the
for-loop and the call to getNextPiece() to use this guard.

Comment on lines +1131 to +1147
// SEGENV.aux1 stores: [bit 15: check2] [bit 14: check3] [bits 10-13: text hash (4 bits)] [bits 0-9: pattern length]
bool lastCheck2 = (SEGENV.aux1 & 0x8000) != 0;
bool lastCheck3 = (SEGENV.aux1 & 0x4000) != 0;
uint16_t lastHashBits = (SEGENV.aux1 >> 10) & 0xF; // 4 bits of hash
uint16_t patternLength = SEGENV.aux1 & 0x3FF; // Lower 10 bits for length (up to 1023)

// Compute text hash
uint16_t textHash = 0;
for (char *p = text; *p; p++) {
textHash = ((textHash << 5) + textHash) + *p;
}
uint16_t currentHashBits = (textHash >> 12) & 0xF; // Use upper 4 bits of hash

bool textChanged = (currentHashBits != lastHashBits) && (SEGENV.call > 0);

// Check if we need to rebuild the pattern
bool needsRebuild = (SEGENV.call == 0) || textChanged || (SEGMENT.check2 != lastCheck2) || (SEGMENT.check3 != lastCheck3);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

A 4-bit rename fingerprint will miss too many name changes.

Using only four hash bits gives roughly a 1-in-16 collision rate, so mode_morsecode() can keep scrolling the previous message after SEGMENT.name changes. Store a larger hash in SEGENV.data or another field instead of truncating it this aggressively.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@usermods/user_fx/user_fx.cpp` around lines 1131 - 1147, The 4-bit hash in
SEGENV.aux1 is too small and causes frequent collisions; compute and keep a
larger fingerprint (e.g., full 16-bit textHash) and persist it in a wider field
(use SEGENV.data or another available SEGENV field) instead of truncating to 4
bits. Change the code that reads lastHashBits from SEGENV.aux1 to read the full
stored hash from SEGENV.data (or the chosen field), compute a 16-bit textHash in
the loop (keep textHash as uint16_t), set currentHash = textHash (or a simple
reduced-but-still-large slice like top 12 bits), compare that against the stored
value when computing textChanged, and ensure when rebuilding you update the
stored hash in SEGENV.data (and stop packing the hash into SEGENV.aux1). Update
references around textHash, currentHashBits/lastHashBits, needsRebuild and where
SEGENV.aux1 was previously written so the new larger hash is saved and loaded
consistently (affects mode_morsecode / SEGMENT.name change detection).

Comment on lines +151 to +158
if (inputPortNum <= (SOC_UART_NUM - 1) && inputPortNum > 0) {
this->inputPortNum = inputPortNum;
}
else {
DEBUG_PRINTF("DMXInput: Error: invalid inputPortNum: %d, default to 1\n", inputPortNum);
this->inputPortNum = 1;
return;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Resource leak on invalid inputPortNum.

When inputPortNum is out of range, the function returns at line 157 after pins have already been allocated (line 143). However, the pins are never deallocated, causing a resource leak—the pins remain reserved but the DMX driver is never initialized.

Consider deallocating the pins before returning, or move the port validation before pin allocation.

Proposed fix: Deallocate pins on early return
     if (inputPortNum <= (SOC_UART_NUM - 1) && inputPortNum > 0) {
       this->inputPortNum = inputPortNum;
     }
     else {
       DEBUG_PRINTF("DMXInput: Error: invalid inputPortNum: %d, default to 1\n", inputPortNum);
-      this->inputPortNum = 1;
+      PinManager::deallocateMultiplePins(pins, 3, PinOwner::DMX_INPUT);
       return;
     }

Alternatively, move the validation before pin allocation:

Alternative fix: Validate before allocation
 void DMXInput::init(uint8_t rxPin, uint8_t txPin, uint8_t enPin, uint8_t inputPortNum)
 {
+  // Validate port number first
+  if (inputPortNum > (SOC_UART_NUM - 1) || inputPortNum == 0) {
+    DEBUG_PRINTF("DMXInput: Error: invalid inputPortNum: %d\n", inputPortNum);
+    return;
+  }

   if (rxPin > 0 && enPin > 0 && txPin > 0) {
     const managed_pin_type pins[] = {
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if (inputPortNum <= (SOC_UART_NUM - 1) && inputPortNum > 0) {
this->inputPortNum = inputPortNum;
}
else {
DEBUG_PRINTF("DMXInput: Error: invalid inputPortNum: %d, default to 1\n", inputPortNum);
this->inputPortNum = 1;
return;
}
if (inputPortNum <= (SOC_UART_NUM - 1) && inputPortNum > 0) {
this->inputPortNum = inputPortNum;
}
else {
DEBUG_PRINTF("DMXInput: Error: invalid inputPortNum: %d, default to 1\n", inputPortNum);
PinManager::deallocateMultiplePins(pins, 3, PinOwner::DMX_INPUT);
return;
}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@wled00/dmx_input.cpp` around lines 151 - 158, The early return on invalid
inputPortNum leaks previously allocated GPIO pins; either validate inputPortNum
before any pin allocation (move the inputPortNum check above the pin-allocation
code) or, if you keep the current order, free/deallocate the allocated pins
before the early return (call the existing pin cleanup routine or add a
deallocatePins()/releasePins() call) so that when the block around
this->inputPortNum sets default and returns the pins are not left reserved.

Comment on lines +82 to +100
void DMXOutput::init(uint8_t outputPin) {
dmx_config_t config = DMX_CONFIG_DEFAULT;
const bool installOk = dmx_driver_install(dmxPort, &config, DMX_INTR_FLAGS_DEFAULT);
if (!installOk) {
DEBUG_PRINTF("DMXOutput: Error: Failed to install dmx driver\n");
return;
}
const bool setPin = dmx_set_pin(dmxPort, outputPin, -1, -1);
if (!setPin) {
DEBUG_PRINTF("DMXOutput: Error: Failed to set DMX output pin\n");
return;
}
}
void DMXOutput::write(uint8_t channel, uint8_t value) {
dmxdata[channel] = value;
}
void DMXOutput::update() {
dmx_write(dmxPort, dmxdata, DMX_PACKET_SIZE);
dmx_send(dmxPort, DMX_PACKET_SIZE);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

find . -type f -name "dmx_output.cpp" -o -name "*dmx*" -type f | head -20

Repository: wled/WLED

Length of output: 198


🏁 Script executed:

cat -n wled00/dmx_output.cpp | head -110 | tail -50

Repository: wled/WLED

Length of output: 1845


🏁 Script executed:

find . -type f \( -name "idf_component.yml" -o -name "idf_component.yaml" -o -name "*.txt" -o -name "*.json" \) | xargs grep -l "esp_dmx" 2>/dev/null | head -5

Repository: wled/WLED

Length of output: 35


🏁 Script executed:

find . -type f \( -name "platformio.ini" -o -name "CMakeLists.txt" -o -name "*.cmake" \) | head -10

Repository: wled/WLED

Length of output: 69


🏁 Script executed:

cat platformio.ini | grep -A 5 -B 5 -i "esp_dmx\|dmx"

Repository: wled/WLED

Length of output: 895


🌐 Web query:

esp_dmx someweisguy commit 47db25d esp_dmx.h dmx_driver_install

💡 Result:

esp_dmx is an open-source ESP32 library by GitHub user someweisguy (likely Mitch Weisbrod) for implementing ANSI-ESTA E1.11 DMX-512A and E1.20 RDM protocols. It supports transmitting/receiving DMX/RDM via any hardware UART port. The header file esp_dmx.h declares dmx_driver_install, the key function to initialize a DMX driver: dmx_driver_install(dmx_port_t dmx_num, const dmx_config_t *config, const dmx_personality_t personalities[], int personality_count); Usage example from README.md (release/v4.1): const dmx_port_t dmx_num = DMX_NUM_1; dmx_config_t config = DMX_CONFIG_DEFAULT; const int personality_count = 1; dmx_personality_t personalities[] = { {.footprint = 1, .slots = 16} }; dmx_driver_install(dmx_num, &config, personalities, personality_count); Then set pins: dmx_set_pin(dmx_num, tx_pin, rx_pin, rts_pin); Commit 47db25d could not be located in searches of the repository. It may be from a fork, private repo, or deleted. No matching commit found in main repo history (2,838 commits noted). Known issues: Compatibility problems with newer ESP-IDF versions (e.g., 5.3+), causing periph_module errors on UART2/LP-UART (ESP32-C6). Fixes proposed in PR #187 and issues #150/#204/#164, but not merged as of 2026-02. Use updated uart.c patches for ESP-IDF 5.4+. Library requires ESP-IDF v4.4.1+ or Arduino-ESP32 v2.0.3+. Latest release v4.1.0 (2024-03-21).

Citations:


🌐 Web query:

esp_dmx dmx_send dmx_send_num API signature v4.1.0

💡 Result:

In the esp_dmx library version 4.1.0 (https://github.com/someweisguy/esp_dmx/tree/release/v4.1), the API functions for sending DMX data are: - dmx_send(dmx_port_t dmx_num): Initiates sending the current DMX buffer to the bus (full packet). - dmx_send_num(dmx_port_t dmx_num, int num_slots): Sends a specified number of slots from the buffer (seen in Arduino example usage). These follow dmx_write(dmx_num, data, size) to populate the buffer. Use dmx_wait_sent(dmx_num, ticks) to wait for completion. Note: esp_dmx is a third-party ESP32 DMX512/RDM library for ESP-IDF/Arduino, not official ESP-IDF. Official ESP-IDF v4.1 has no esp_dmx component. Library requires ESP-IDF v4.4.1+, but v4.1 branch exists with recent compatibility fixes (PR #187). Arduino examples confirm dmx_send_num(dmxPort, DMX_PACKET_SIZE).

Citations:


🏁 Script executed:

# Let's verify the commit exists by checking if it's in a specific branch
cd . && git log --oneline | grep "47db25d" || echo "Commit not in current history"

Repository: wled/WLED

Length of output: 82


Fix API calls to match esp_dmx v4.1.0 signatures.

The pinned esp_dmx library (commit 47db25d) requires four arguments for dmx_driver_install(dmx_num, config, personalities[], personality_count) and one argument for dmx_send(dmx_num). The current code passes incorrect argument counts and types:

  • Line 84: dmx_driver_install(dmxPort, &config, DMX_INTR_FLAGS_DEFAULT) has 3 args; the third should be a personalities array pointer and a fourth argument specifying the count
  • Line 100: dmx_send(dmxPort, DMX_PACKET_SIZE) has 2 args but only one is accepted
Fix
 void DMXOutput::init(uint8_t outputPin) {
   dmx_config_t config = DMX_CONFIG_DEFAULT;
-  const bool installOk = dmx_driver_install(dmxPort, &config, DMX_INTR_FLAGS_DEFAULT);
+  const bool installOk = dmx_driver_install(dmxPort, &config, nullptr, 0);
     if (!installOk) {
       DEBUG_PRINTF("DMXOutput: Error: Failed to install dmx driver\n");
       return;
     }
   const bool setPin = dmx_set_pin(dmxPort, outputPin, -1, -1);
@@ void DMXOutput::update() {
   dmx_write(dmxPort, dmxdata, DMX_PACKET_SIZE);
-  dmx_send(dmxPort, DMX_PACKET_SIZE);
+  dmx_send(dmxPort);
 }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@wled00/dmx_output.cpp` around lines 82 - 100, The DMX API calls use the
v4.1.0 signatures: replace the current three-arg dmx_driver_install call in
DMXOutput::init with the four-arg form expected by esp_dmx (pass dmxPort, config
(not &config), a personalities pointer (use nullptr or an empty array) and a
personality count (0) to satisfy the new signature), and change the dmx_send
call in DMXOutput::update to the single-argument form dmx_send(dmxPort); keep
using dmx_write(dmxPort, dmxdata, DMX_PACKET_SIZE) and dmx_set_pin(dmxPort,
outputPin, -1, -1) as before but ensure the types match the updated API.

Comment on lines +1165 to +1188
// validate the bus config: count I2S buses and check if they meet requirements
unsigned i2sBusCount = 0;

for (const auto &bus : busConfigs) {
if (Bus::isDigital(bus.type) && !Bus::is2Pin(bus.type)) {
digitalCount++;
if (busType == 0) busType = bus.type; // remember first bus type
if (busType != bus.type) {
DEBUG_PRINTF_P(PSTR("Mixed digital bus types detected! Forcing single I2S output.\n"));
useParallelI2S = false; // mixed bus types, no parallel I2S
}
if (bus.count > maxLedsOnBus) maxLedsOnBus = bus.count;
if (bus.driverType == 1)
i2sBusCount++;
}
}
DEBUG_PRINTF_P(PSTR("Maximum LEDs on a bus: %u\nDigital buses: %u\n"), maxLedsOnBus, digitalCount);
// we may remove 600 LEDs per bus limit when NeoPixelBus is updated beyond 2.8.3
if (maxLedsOnBus <= 600 && useParallelI2S) BusManager::useParallelOutput(); // must call before creating buses
else useParallelI2S = false; // enforce single I2S
digitalCount = 0;
DEBUG_PRINTF_P(PSTR("Digital buses: %u, I2S buses: %u\n"), digitalCount, i2sBusCount);

// Determine parallel vs single I2S usage (used for memory calculation only)
bool useParallelI2S = false;
#if defined(CONFIG_IDF_TARGET_ESP32S3)
// ESP32-S3 always uses parallel LCD driver for I2S
if (i2sBusCount > 0) {
useParallelI2S = true;
}
#else
if (i2sBusCount > 1) {
useParallelI2S = true;
}
#endif
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

The new RAM gate can placeholder buses that actually fit.

i2sBusCount is derived from driverType before BusManager::getI() resolves any fallback to RMT, so useParallelI2S can stay enabled for a bus set that is no longer actually parallel-I2S. Then Line 1225 adds getBusSize() on top of BusConfig::memUsage(), which already counted the bus object + driver buffer. That inflates mem twice and makes use_placeholder fire too early.

Possible direction
-  unsigned i2sBusCount = 0;
-
-  for (const auto &bus : busConfigs) {
-    if (Bus::isDigital(bus.type) && !Bus::is2Pin(bus.type)) {
-      digitalCount++;
-      if (bus.driverType == 1)
-        i2sBusCount++;
-    }
-  }
+  unsigned i2sBusCount = 0;
+  for (auto &bus : busConfigs) {
+    if (Bus::isDigital(bus.type) && !Bus::is2Pin(bus.type)) digitalCount++;
+    bus.iType = BusManager::getI(bus.type, bus.pins, bus.driverType);
+    if (Bus::isDigital(bus.type) && !Bus::is2Pin(bus.type) && (bus.iType & 0x01) == 0) i2sBusCount++;
+  }

-  for (auto &bus : busConfigs) {
-    // assign bus types...
-    bus.iType = BusManager::getI(bus.type, bus.pins, bus.driverType);
-  }
   for (auto &bus : busConfigs) {
     bool use_placeholder = false;
     unsigned busMemUsage = bus.memUsage();
     mem += busMemUsage;
     ...
-    if (BusManager::add(bus, use_placeholder) != -1) {
-      mem += BusManager::busses.back()->getBusSize();
+    if (BusManager::add(bus, use_placeholder) != -1) {
       if (Bus::isDigital(bus.type) && !Bus::is2Pin(bus.type) && BusManager::busses.back()->isPlaceholder()) digitalCount--;
     }
   }

Also applies to: 1193-1226

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@wled00/FX_fcn.cpp` around lines 1165 - 1188, The code counts i2sBusCount from
raw BusConfig.driverType before fallbacks and then later double-counts bus
memory (memUsage + getBusSize), causing use_placeholder to trigger incorrectly;
fix by (1) moving the I2S detection to use the resolved runtime driver (call
BusManager::getI(...) or resolve the bus via BusManager before counting) and
derive useParallelI2S from that resolved driver instead of BusConfig.driverType,
and (2) remove the duplicate size addition (do not add getBusSize() on top of
BusConfig::memUsage() or adjust memUsage so it only counts each bus once) so
that mem and use_placeholder are computed from the same, non-duplicated values
(refer to busConfigs, i2sBusCount, Bus::isDigital, Bus::is2Pin, driverType,
BusManager::getI, useParallelI2S, BusConfig::memUsage, getBusSize,
use_placeholder).

Comment on lines 56 to +66
const wled_metadata_t __attribute__((section(BUILD_METADATA_SECTION))) WLED_BUILD_DESCRIPTION = {
WLED_CUSTOM_DESC_MAGIC, // magic
WLED_CUSTOM_DESC_VERSION, // version
/*WLED_CUSTOM_DESC_VERSION*/ 1, // structure version. Currently set to 1 to allow OTA from broken original version. FIXME before 0.16 release.
TOSTRING(WLED_VERSION),
WLED_RELEASE_NAME, // release_name
std::integral_constant<uint32_t, djb2_hash_constexpr(WLED_RELEASE_NAME)>::value, // hash - computed at compile time; integral_constant enforces this
#if defined(ESP32) && defined(CONFIG_IDF_TARGET_ESP32)
{ 0, 15, 3 }, // Some older ESP32 might have bootloader issues; assume we'll have it sorted by 0.15.3
#else
{ 0, 15, 2 }, // All other platforms can update safely
#endif
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Emit the actual v2 metadata version here, or the safe-update gate never runs.

shouldAllowOTA() only checks safe_update_version when the uploaded image reports desc_version > 1. With Line 58 hard-coded to 1, every binary built from this branch bypasses the new guard, so the protection stays disabled until someone remembers to remove the FIXME.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@wled00/wled_metadata.cpp` around lines 56 - 66, The metadata struct
WLED_BUILD_DESCRIPTION is emitting a hard-coded version of 1 which prevents
shouldAllowOTA() from checking safe_update_version; change the second field
(currently "/*WLED_CUSTOM_DESC_VERSION*/ 1") to the actual v2 metadata version
(e.g., replace with WLED_CUSTOM_DESC_VERSION or the literal 2) so binaries
produced include desc_version > 1 and trigger the safe-update gate (update the
initializer in WLED_BUILD_DESCRIPTION).

Comment on lines +131 to +137
template<size_t len>
static inline String bufToString(const char (&buf)[len]) {
char sbuf[len+1];
size_t real_len = strnlen(buf, len);
memcpy(sbuf, buf, real_len);
sbuf[len] = '\0';
return sbuf;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Terminate the copied string at real_len.

When the source name is shorter than the fixed buffer, sbuf[real_len..len-1] stays uninitialized and String(sbuf) reads past the copied text until it happens to find '\0'. That makes OTA release-name comparisons nondeterministic.

💡 Minimal fix
 template<size_t len>
 static inline String bufToString(const char (&buf)[len]) {
-  char sbuf[len+1];
+  char sbuf[len + 1];
   size_t real_len = strnlen(buf, len);
   memcpy(sbuf, buf, real_len);
-  sbuf[len] = '\0';
+  sbuf[real_len] = '\0';
   return sbuf;
 }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@wled00/wled_metadata.cpp` around lines 131 - 137, The bufToString template
leaves sbuf uninitialized past real_len causing nondeterministic reads; in the
bufToString(const char (&buf)[len]) function, after memcpy(..., real_len) set
sbuf[real_len] = '\0' (instead of sbuf[len]) so the copied string is properly
null-terminated and String(sbuf) only reads the intended bytes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Awaiting testing connectivity Issue regarding protocols, WiFi connection or availability of interfaces

Projects

None yet

Development

Successfully merging this pull request may close these issues.